Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

[SecuritySolution] Dashboard listing UI #160540

Merged
merged 41 commits into from
Jul 24, 2023
Merged

Conversation

angorayc
Copy link
Contributor

@angorayc angorayc commented Jun 26, 2023

Summary

  1. Align dashboard listing UI with Kibana dashboard.
  2. Security Solution tags are selected by default and removable by users.

Prerequisite:
This PR is waiting for #160871 to be merged

Steps to verify:

  1. Visit Security > Dashboards, and create a dashboard from this page.
  2. Back to Security Dashboards page, you should see the dashboard you just created and Security Solution tag should be selected by default in the tag filters.
  3. Open the tag options, click the Security Solution tag. Observe that it should be removable, and it should display all the dashboards you have in the table.

Known issues:
#160540 (comment)

Before:

Screenshot 2023-06-27 at 09 24 19

After:
Screenshot 2023-06-27 at 09 22 21

Checklist

Delete any items that are not applicable to this PR.

Comment on lines 66 to 77
const totalActiveFilters = useMemo(
() =>
Object.keys(tagSelection).reduce((acc, currentOption) => {
const inTagReferences = tagReferences?.find((ref) => ref.name === currentOption);
acc +=
inTagReferences != null
? tagReferences?.filter((ref) => ref.name === currentOption).length ?? 0
: 1;
return acc;
}, 0),
[tagReferences, tagSelection]
);
Copy link
Contributor Author

@angorayc angorayc Jun 26, 2023

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This is to fix this scenario: (When more than one tags with the same name exist, it show the count as 1, expected to be 2)
incorrect_tag_counts

@angorayc angorayc added release_note:skip Skip the PR/issue when compiling release notes Team:Threat Hunting Security Solution Threat Hunting Team Team: SecuritySolution Security Solutions Team working on SIEM, Endpoint, Timeline, Resolver, etc. Team:Threat Hunting:Explore Feature:Security Dashboards Security solution custom dashboards feature v8.10.0 labels Jun 27, 2023
@angorayc angorayc changed the title Dashboard listing [SecuritySolution] Dashboard listing UI Jun 27, 2023
@angorayc angorayc marked this pull request as ready for review June 27, 2023 08:33
@angorayc angorayc requested review from a team as code owners June 27, 2023 08:33
@elasticmachine
Copy link
Contributor

Pinging @elastic/security-threat-hunting (Team:Threat Hunting)

@elasticmachine
Copy link
Contributor

Pinging @elastic/security-solution (Team: SecuritySolution)

Copy link
Contributor

@sebelga sebelga left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thanks for working on this @angorayc

I am sorry but I need to push back changes related to "layout". You've added 3 new props for it:

  • withPageTemplateHeader
  • restrictPageSectionWidth
  • pageSectionPadding

which are all related to changes in layout.

Since #157988 there is a <TableListViewTable /> that is exposed and only contains the Table which seems to be what you need.

It would probably means that the DashboardListing should be refactored to only export the Table for Dashboard as well.

If you want we can discuss over Zoom 👍

Copy link
Contributor

@sebelga sebelga left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Left some comments

http,
},
toMountPoint,
savedObjectsTagging: savedObjectsTagging.hasApi // TODO: clean up this logic once https://github.com/elastic/kibana/issues/140433 is resolved
Copy link
Contributor

@sebelga sebelga Jul 3, 2023

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Copy link
Contributor

@sebelga sebelga left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Start to look good! 👍 One more change from my side to fix a big with the tag list. Cheers

Copy link
Contributor

@ThomThomson ThomThomson left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Presentation team changes LGTM! I tested locally and by looking through the code, and everything is working great! Nice work with splitting the Listing up into two components.

One thing that I noticed is that the Security Dashboard listing page has two Create dashboard buttons - is this expected?

Also left one small nit about the jest testing.

},
}));

const mockGetServices = {
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

nit: some of these services look like stubs which should already be filled out by our stubs in src/plugins/dashboard/jest_setup.ts. Usually we only override services like this when we need to test a particular behaviour - and in that case it's actually possible to simply override the stub function.

For instance, the default stub for the dashboardSessionStorage.getDashboardIdsWithUnsavedChanges service defined here: src/plugins/dashboard/public/services/dashboard_session_storage/dashboard_session_storage.stub.ts returns two dashboards. If you wanted to test what happens if it returns three you could just do

pluginServices.getServices().dashboardSessionStorage.getDashboardIdsWithUnsavedChanges = jest.fn().mockReturnValue([ 'one', 'two', 'three' ]);

In this way we can avoid having to restate all required services for every test. This isn't blocking though, would just help the test be a little less verbose!

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

We have an existing create dashboard button on the page and a new one in dashboard listing. I'll leave it as it is at the moment, as users might have used to the position of the existing button.

Screenshot 2023-07-18 at 10 08 50

I've also updated the unit test and re-use the existing mocks. Thanks for the tips @ThomThomson !

Copy link
Contributor

@sebelga sebelga left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

LGTM! 👍 Thanks for the patience and making those changes @angorayc !


const savedObjectsTaggingFakePlugin = useMemo(
() =>
savedObjectsTagging.hasApi // TODO: clean up this logic once https://github.com/elastic/kibana/issues/140433 is resolved
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Issue 140433 is closed. Is this comment outdated?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Nope, this comment is still valid - the linked issue is just the wrong way to solve it!

@machadoum
Copy link
Member

nit: I noticed it takes some seconds for the Dashboards table to load. Having a loading state/skeleton might be helpful for the user so they know that something is still loading.

Jul-20-2023.12-18-42.mov

I also noticed that the new tag never gets loaded when the know issue happens and you navigate using the menu.

Jul-20-2023.12-25-24.mov

When I create a new dashboard it also doesn't show up:

new.dashboard.is.hidden.mov

The tag count is also wrong to me...
Screenshot 2023-07-20 at 12 35 39

Am I doing something wrong? 🤔

@kibana-ci
Copy link
Collaborator

💚 Build Succeeded

Metrics [docs]

Module Count

Fewer modules leads to a faster build time

id before after diff
dashboard 384 386 +2
securitySolution 4314 4311 -3
total -1

Async chunks

Total size of all lazy-loaded chunks that will be downloaded as the user navigates the app

id before after diff
dashboard 355.7KB 358.2KB +2.5KB
securitySolution 9.8MB 9.8MB -1.3KB
total +1.2KB

Page load bundle

Size of the bundles that are downloaded on every page load. Target size is below 100kb

id before after diff
dashboard 42.4KB 42.4KB +13.0B
Unknown metric groups

async chunk count

id before after diff
dashboard 11 12 +1

ESLint disabled line counts

id before after diff
dashboard 7 8 +1

Total ESLint disabled count

id before after diff
dashboard 7 8 +1

History

To update your PR or re-run it, just comment with:
@elasticmachine merge upstream

@angorayc
Copy link
Contributor Author

@machadoum , thank you so much for testing this PR. I've added a loading icon and implemented the recommended solution of #160723. Please have another try and see if it still happening, thanks again!

Copy link
Member

@machadoum machadoum left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I tried it out once more and it's working splendidly. Thank you, @angorayc!

@angorayc angorayc added the backport:skip This commit does not require backporting label Jul 24, 2023
@angorayc angorayc merged commit 50a9e13 into elastic:main Jul 24, 2023
ThomThomson pushed a commit to ThomThomson/kibana that referenced this pull request Aug 1, 2023
## Summary


1. Align dashboard listing UI with Kibana dashboard.
2. `Security Solution` tags are selected by default and removable by
users.

**Prerequisite:**
This PR is waiting for elastic#160871 to
be merged


**Steps to verify:**
1. Visit Security > Dashboards, and create a dashboard from this page.
2. Back to Security Dashboards page, you should see the dashboard you
just created and Security Solution tag should be selected by default in
the tag filters.
3. Open the tag options, click the Security Solution tag. Observe that
it should be removable, and it should display all the dashboards you
have in the table.

**Known issues:**
elastic#160540 (comment)

**Before:**

<img width="2545" alt="Screenshot 2023-06-27 at 09 24 19"
src="https://github.com/elastic/kibana/assets/6295984/bc0fa0b1-96ad-43b0-afc1-48444dfb5691">


**After:**
<img width="2543" alt="Screenshot 2023-06-27 at 09 22 21"
src="https://github.com/elastic/kibana/assets/6295984/82d0a868-bda2-431f-b0b5-9cbc34d3ae71">


### Checklist

Delete any items that are not applicable to this PR.

- [x] Any text added follows [EUI's writing
guidelines](https://elastic.github.io/eui/#/guidelines/writing), uses
sentence case text and includes [i18n
support](https://github.com/elastic/kibana/blob/main/packages/kbn-i18n/README.md)
- [ ]
[Documentation](https://www.elastic.co/guide/en/kibana/master/development-documentation.html)
was added for features that require explanation or tutorials
- [x] [Unit or functional
tests](https://www.elastic.co/guide/en/kibana/master/development-tests.html)
were updated or added to match the most common scenarios

---------

Co-authored-by: kibanamachine <[email protected]>
Co-authored-by: Pablo Neves Machado <[email protected]>
@angorayc
Copy link
Contributor Author

#164476

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
backport:skip This commit does not require backporting Feature:Security Dashboards Security solution custom dashboards feature needs_docs release_note:skip Skip the PR/issue when compiling release notes Team: SecuritySolution Security Solutions Team working on SIEM, Endpoint, Timeline, Resolver, etc. Team:Threat Hunting:Explore Team:Threat Hunting Security Solution Threat Hunting Team v8.10.0
Projects
None yet
Development

Successfully merging this pull request may close these issues.

8 participants